Refactor function return type representation #6463
Conversation
This separates the return type from the return pattern, and replaces the return pattern with a block of return patterns. This is a step toward support for `ref` returns (where there's no corresponding return pattern) and compund-form returns (where there may be multiple return patterns).
chandlerc
left a comment
chandlerc
left a comment
There was a problem hiding this comment.
Generally nice -- especially nice that our type location gets better even if the C++ imported return type location is ... even more wrong. =] The TODOs make sense though, thanks for those. Some comments / questions inline:
| // | ||
| // TODO: We only form these as the instruction referenced by a `NameRef`. | ||
| // Consider merging an `SpecificConstant` + `NameRef` into a new form of | ||
| // instruction in order to give a more compact representation. |
There was a problem hiding this comment.
Is this an unrelated change? Maybe I'm missing how it connects...
There was a problem hiding this comment.
The connection is that in thunk.cpp I'm forming a SpecificConstant that's not referenced by a NameRef, so the premise of this TODO is no longer true (in fact, it wasn't true before this PR either, but that's no reason to leave it here).
| // The type inst representing the function's explicitly declared return type, | ||
| // if any. |
There was a problem hiding this comment.
Maybe this should be:
| // The type inst representing the function's explicitly declared return type, | |
| // if any. | |
| // The type inst representing the function's return type, as computed from | |
| // an explicit return pattern (if any). |
Or maybe s/pattern/form/ ?
Just trying to get at the idea that the type may not be what is explicit.
There was a problem hiding this comment.
Right now, the type is what's explicit, because we don't yet support return forms, or even ref returns. Of course, this comment will be wrong later when we add support for those things, but at that point the field's name and type will probably also be wrong, so it seems simplest to update all three together.
Also, "as computed from" suggests to me that this inst is the result of constant-evaluating the return type expression, but this has to be the inst that represents the return type expression the user actually wrote, because it's what we use when we need a LocId pointing to that expression.
toolchain/check/cpp/import.cpp
Outdated
| } // namespace | ||
|
|
||
| // Creates a block containing the parameter pattern instructions for the | ||
| // explicit parameters, a parameter pattern instruction for the return type and |
There was a problem hiding this comment.
Is the "a parameter pattern instruction for the return type" still accurate? Seems like its in the return pattern block now?
Or maybe this is saying something different, sorry if I've misunderstood this code...
There was a problem hiding this comment.
Yeah, this needs to be updated. How's this?
| function_lowering.SetLocal(param_id, param_value); | ||
| }; | ||
|
|
||
| // Lower the return slot parameter. |
There was a problem hiding this comment.
Should this comment be updated?
Unclear if this is talking about the function.return_slot_pattern_id sense of "return slot parameter", or if it's talking about the LLVM return slot parameter...
There was a problem hiding this comment.
Should this comment be updated?
Unclear if this is talking about the
function.return_slot_pattern_idsense of "return slot parameter", or if it's talking about the LLVM return slot parameter...
I read it as referring to the LLVM return slot parameter (that's clearly what it means on lines 693 and 701), so I think it should probably stay as-is for now.
I did notice that the comment on line 694 needs to be updated, though.
2 participants
This separates the return type from the return pattern, and replaces the return pattern with a block of return patterns. This is a step toward support for
refreturns (where there's no corresponding return pattern) and compund-form returns (where there may be multiple return patterns).